Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add tf2 Buffer accessor in FrameManager #1224

Conversation

IanTheEngineer
Copy link

@IanTheEngineer IanTheEngineer commented Apr 20, 2018

As this modifies headers (though only adding API), this may need to target melodic-devel.

This PR would mitigate the Issue related to using tf2 in the FrameManager #1215 by creating a tf2_ros TransformListener and Buffer for use by those who wish to use the RViz API and rely solely on the tf2 libraries. In particular, MoveIt! is in the middle of a migration to tf2 ( moveit/moveit#830 ) and this PR would unblock that migration.

@IanTheEngineer
Copy link
Author

@dhood @wjwwood any chance you could give this PR a lookover?

The benefits include allowing users of RViz' API to transition away from the deprecated tf library, toward tf2, by utilizing a shared Buffer object.

As far as I can tell, with this implementation, the only downside is that there lacks an option to re-use a tf2_ros::Buffer passed in as an argument to the constructor, but I can add that if it is desired.

@IanTheEngineer IanTheEngineer force-pushed the tf2_buffer branch 2 times, most recently from 6e52963 to f347dd6 Compare April 23, 2018 21:22
@dirk-thomas
Copy link
Contributor

@wjwwood Can you please take a look at this since it is necessary for the migration of MoveIt to tf2. Thanks.

@dhood
Copy link
Contributor

dhood commented Apr 26, 2018

you are right that this will break ABI and will need to target melodic.

the downside that I see is that we now have duplicated the listener effort for TF, which is a shame since the tf1 listener is actually using a tf2 buffer underneath. I know you had worked to expose it in #1215 (comment); did you consider that approach here already?

@IanTheEngineer
Copy link
Author

@dhood I agree that reusing the tf listener would be ideal, but the underlying tf2_ros::Buffer object is not designed to be accessible outside of inheretance.
I ran into slicing issues in attempting to retrieve the pre-alocated & protected tf_buffer object and maintain its access inside a shared pointer, without it being deleted from underneath the shared_ptr and causing a coredump. In the end, I figure the best solution is probably the simplest (implemented in this PR) even if it is not the most efficient. However, I am open to recommendations on a better implementation (or pointers as to how my C++ foo is failing).

@dhood
Copy link
Contributor

dhood commented Apr 26, 2018

@wjwwood could you pitch in on this please if you get a chance? (and the split std::shared_ptr and boost::shared_ptr interface: maybe stick to boost for consistency in the API?)

@wjwwood
Copy link
Member

wjwwood commented Apr 26, 2018

Yeah, I was hoping to do it today. We'll see...

@dhood dhood added this to the first melodic release milestone Apr 26, 2018
@wjwwood
Copy link
Member

wjwwood commented Apr 28, 2018

I'd really rather not have duplicated tf buffers. So I'm going to pursue why the subclass didn't work and/or adding a new method in tf that would give access to the underlying tf2::Buffer without subclassing.

#1215 (comment)

@wjwwood
Copy link
Member

wjwwood commented May 9, 2018

Closing since I believe this was worked around in ros/geometry#163. Please comment if that's not the case.

@wjwwood wjwwood closed this May 9, 2018
@IanTheEngineer
Copy link
Author

Thanks @wjwwood! I meant to close it myself. I believe this PR is unneeded due to the accessor method added to ros/geometry#163 Thanks for the reviews :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants